Various router fixes and #2417 follow-ups by tnull · Pull Request #2575 · lightningdevkit/rust-lightning

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull · 2023-09-14T14:48:10Z

Based on #2570.

When overpaying to meet htlc_minimum_msat, we update the path value and recompute fees accordingly. However, so far we didn't account for the extra paid fees in value_contribution_msat, which leads to it being
slightly off, potentially having us hit a debug assertion when later checking the route's total value matches
already_collected_value_msat. Here, we therefore add the extra fees to value_contribution_msat.

Moreover, we switch a calculation updating the used liquidity to use saturating_add, as it could potentially overflow and panic in debug.

We also include a number of follow-ups for #2417.

tnull · 2023-09-14T15:18:21Z

Put in draft while taking another look at the failing test.

TheBlueMatt

Would be nice to get a test for this, but LGTM.

TheBlueMatt · 2023-09-19T01:31:26Z

Feel free to squash the fixup.

tnull · 2023-09-19T07:10:52Z

Feel free to squash the fixup.

Squashed without further changes for now.

codecov-commenter · 2023-09-22T14:53:44Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7b4fb9d) 88.98% compared to head (be1088a) 89.00%.
Report is 4 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2575 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02% 
==========================================
Files 113 113 Lines 86291 86897 +606 Branches 86291 86897 +606 ==========================================
+ Hits 76787 77345 +558 - Misses 7267 7300 +33 - Partials 2237 2252 +15

Files	Coverage Δ
lightning/src/ln/payment_tests.rs	`98.20% <100.00%> (+0.07%)`	⬆️
lightning/src/routing/router.rs	`93.85% <100.00%> (+<0.01%)`	⬆️
lightning/src/util/test_utils.rs	`76.96% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/outbound_payment.rs	`88.56% <69.23%> (-0.36%)`	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheBlueMatt

This is missing test coverage for the various fixes, which would be really nice to include given the possibility of regressions.

valentinewallace · 2023-09-27T19:40:56Z

+ // Only add the hops in this route to our candidate set if either we are part of
+ // the first hop, we have a direct channel to the first hop, or the first hop is in
+ // the regular network graph.
+ our_node_id == first_hop_src_id ||


Would be nice to get test coverage of this at some point

Agreed, unfortunately not sure if it will make it in this PR though, if we want to land it for 117.

Previously this calculation could overflow, leading to panicking in `debug`.

tnull · 2023-09-28T09:22:04Z

Rebased on main, fixed the route_params handling in a bunch of tests and dropped the cmp::min and now just calculate the retry final_value_msat by deducting the sum of the succeeded paths' values from the previous final_value_msat.

TheBlueMatt · 2023-09-28T17:18:39Z

LGTM, feel free to squash, I think.

valentinewallace

LGTM after squash

Previously, we would only consider route hints if we had a direct channel to the first node in the hint or if the first node in the hint was part of the public network graph. However, this left out the possiblity of us being part of the first hop, especially if our own node is not announced and part of the graph.

If we have a direct channel to a node generating an invoice with route hints, we'd previously happily add multiple candidates that all refer to the same channel. To keep our candidate set small and unify our tracking where possible, we now check if its `short_channel_id` is an `outbound_scid_alias` of any of our first hops and refrain from adding another candidate if it's the case.

.. as a follow-up from lightningdevkit#2417.

Previously, if an overpaid path would fail immediately, we'd retry a `PartialFailure` with the full path amount, _including_ any overpayment. Here, we now subtract the succeeded paths' values from the net. value to exclude the overpaid amounts on retry.

tnull · 2023-09-28T17:46:15Z

Squashed fixups and included following changes:

> git diff-tree -U2 dbad0824 be1088acdiff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 4252cd40..193fe352 100644
--- a/lightning/src/routing/router.rs+++ b/lightning/src/routing/router.rs@@ -2143,7 +2143,6 @@ where L::Target: Logger {
.filter(|route| !route.0.is_empty())
{
- let first_hop_in_route = &(route.0)[0];- let first_hop_src_id = NodeId::from_pubkey(&first_hop_in_route.src_node_id);- let have_hop_src_in_graph =+ let first_hop_src_id = NodeId::from_pubkey(&route.0.first().unwrap().src_node_id);+ let first_hop_src_is_reachable =
// Only add the hops in this route to our candidate set if either we are part of
// the first hop, we have a direct channel to the first hop, or the first hop is in
@@ -2152,5 +2151,5 @@ where L::Target: Logger {
first_hop_targets.get(&first_hop_src_id).is_some() ||
network_nodes.get(&first_hop_src_id).is_some();
- if have_hop_src_in_graph {+ if first_hop_src_is_reachable {
// We start building the path from reverse, i.e., from payee
// to the first RouteHintHop in the path.

Improve test coverage of #2575 router fixes

tnull force-pushed the 2023-09-fix-debug-panic branch from 2544d6b to b51e86eCompare September 14, 2023 14:56

tnull marked this pull request as draft September 14, 2023 15:11

TheBlueMatt reviewed Sep 14, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

tnull force-pushed the 2023-09-fix-debug-panic branch from b51e86e to 8d4490cCompare September 15, 2023 07:25

tnull marked this pull request as ready for review September 15, 2023 07:29

TheBlueMatt reviewed Sep 17, 2023
View reviewed changes

TheBlueMatt added this to the 0.0.117 milestone Sep 17, 2023

tnull mentioned this pull request Sep 18, 2023
Fix router fuzz failure #2570
Merged

tnull force-pushed the 2023-09-fix-debug-panic branch from 8d4490c to 50c9dbaCompare September 19, 2023 07:10

tnull force-pushed the 2023-09-fix-debug-panic branch from 50c9dba to 85da1a1Compare September 22, 2023 14:53

tnull force-pushed the 2023-09-fix-debug-panic branch 3 times, most recently from dec0e12 to 0373099Compare September 22, 2023 15:44

TheBlueMatt reviewed Sep 22, 2023
View reviewed changes

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

tnull force-pushed the 2023-09-fix-debug-panic branch from 0373099 to 15a18b8Compare September 25, 2023 12:59

TheBlueMatt reviewed Sep 25, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

tnull force-pushed the 2023-09-fix-debug-panic branch 3 times, most recently from 4a2d62a to fcd9fcaCompare September 26, 2023 10:30

valentinewallace reviewed Sep 26, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt mentioned this pull request Sep 26, 2023
Try to overpay the recipient if we fail to find a path at all and limit overpay #2604
Merged

tnull force-pushed the 2023-09-fix-debug-panic branch from fcd9fca to 4478fbdCompare September 27, 2023 07:58

tnull mentioned this pull request Sep 27, 2023
Add config option to set maximum total routing fee #2417
Merged

tnull commented Sep 27, 2023
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs Outdated

tnull mentioned this pull request Sep 27, 2023
#2417 followups #2600
Closed

tnull force-pushed the 2023-09-fix-debug-panic branch from 2496db9 to f39790dCompare September 27, 2023 14:54

tnull changed the title ~~Router: Account for extra fees in value contribution~~Various router fixes and #2339 follow-upsSep 27, 2023

tnull changed the title ~~Various router fixes and #2339 follow-ups~~Various router fixes and #2417 follow-upsSep 27, 2023

valentinewallace reviewed Sep 27, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs

tnull force-pushed the 2023-09-fix-debug-panic branch from d7ab0ef to aa6a612Compare September 27, 2023 19:14

valentinewallace reviewed Sep 27, 2023
View reviewed changes

TheBlueMatt reviewed Sep 27, 2023
View reviewed changes

Comment threadlightning/src/ln/payment_tests.rs

tnull added 2 commits September 28, 2023 10:14

Use saturating_add when adding inflight HTLCs values
391da3f
Previously this calculation could overflow, leading to panicking in `debug`.

Extend logging of ignored candidates
a8eef29

tnull force-pushed the 2023-09-fix-debug-panic branch 4 times, most recently from 5a5f7d1 to 98c9849Compare September 28, 2023 09:13

valentinewallace reviewed Sep 28, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

valentinewallace reviewed Sep 28, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

tnull added 6 commits September 28, 2023 19:45

Make ignored candidate counts u32
39c5bbc
.. as a follow-up from lightningdevkit#2417.

Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
fb2c959
.. as a follow-up from lightningdevkit#2417.

Assert query's and route's final_value_msat are equal
8c99e34

tnull force-pushed the 2023-09-fix-debug-panic branch from dbad082 to be1088aCompare September 28, 2023 17:45

valentinewallace approved these changes Sep 28, 2023
View reviewed changes

TheBlueMatt approved these changes Sep 28, 2023
View reviewed changes

TheBlueMatt merged commit 1e6707d into lightningdevkit:mainSep 28, 2023

tnull mentioned this pull request Sep 29, 2023
Improve test coverage of #2575 router fixes #2625
Merged

tnull mentioned this pull request Oct 8, 2023
Explicitly reject routes which double back #2215
Closed

TheBlueMatt added a commit that referenced this pull request Oct 15, 2023
Merge pull request#2625from tnull/2023-09-moar-tests-n-fixes
c86cacd
Improve test coverage of #2575 router fixes