Add config option to set maximum total routing fee by tnull · Pull Request #2417 · lightningdevkit/rust-lightning

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull · 2023-07-14T15:08:26Z

Fixes#2339.
Closes#2596.
~~Based on #2555.~~

Currently, users have no means to upper-bound the total fees accruing when finding a route. Here, we add a corresponding field to PaymentParameters which is used to limit the candidate set during path finding.

To this end, we exclude any candidate hops if we find that using them would let the aggregated path routing fees exceed max_total_routeing_fee_msat. Moreover, we return an error if the aggregated fees over all paths of the selected route would surpass max_total_routeing_fee_msat.

tnull · 2023-07-14T15:17:16Z

Drafting for now as I'm not super happy that we only filter on a per-path basis and then fail entirely if the aggregate route surpasses the routing fee limit. So far experimented with the approach we took for the minimal_value_contribution, i.e., having static per-path quotas, but discarded this as it would for example limit the per-path total fee to max_total_routing_fee_msat/max_path_count, i.e., to only 10% of the given value by default.

I think it's not super bad currently as we already pre-sort and filter routes by their cost/value ratio, which however also includes the penalty. Considering whether it would make sense to, whenever max_total_routing_fee_msat is set, add yet another pre-filtering step that only takes total fees into account to decrease the chances of unnecessarily returning an empty route.

This also currently doesn't account for retries. We might need to track the 'used' fees in a structure similar to InFlightHtlcs.

codecov-commenter · 2023-07-14T16:14:00Z

Codecov Report

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

Comparison is base (7e7e7a0) 88.83% compared to head (77afeac) 89.68%.
Report is 35 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 #2417 +/- ##
==========================================
+ Coverage 88.83% 89.68% +0.85% 
==========================================
Files 113 113 Lines 84489 89980 +5491 Branches 84489 89980 +5491 ==========================================
+ Hits 75052 80696 +5644 + Misses 7205 7055 -150 + Partials 2232 2229 -3

Files	Coverage Δ
lightning/src/ln/blinded_payment_tests.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/functional_test_utils.rs	`93.62% <ø> (+2.45%)`	⬆️
lightning/src/ln/channelmanager.rs	`87.68% <50.00%> (+6.55%)`	⬆️
lightning-invoice/src/payment.rs	`77.30% <33.33%> (-0.26%)`	⬇️
lightning/src/ln/payment_tests.rs	`98.06% <98.60%> (+0.01%)`	⬆️
lightning/src/ln/outbound_payment.rs	`90.03% <92.94%> (+1.23%)`	⬆️
lightning/src/routing/router.rs	`93.73% <93.22%> (+0.10%)`	⬆️

... and 17 files with indirect coverage changes

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

TheBlueMatt

This is fine, I think. I agree we should clean it up later to more aggressively find paths even though our first choice was fee-limited out, but this is certainly better than not having fee limiting.

valentinewallace

Looks good to me as a first step, given where we're at with the release!

alecchendev

Nice

valentinewallace · 2023-07-18T20:31:53Z

LGTM after squash

tnull · 2023-07-18T20:41:17Z

LGTM after squash

Squashed fixups without further changes.

tnull · 2023-07-20T13:46:18Z

Rebased on main to get rid of the CI failure.

TheBlueMatt

We should land this to get the API in place for a second alpha, but we should get some of these comments fixed (at least the total-fee final check one) for 0.0.117.

TheBlueMatt · 2023-09-26T18:37:14Z

+ // subtracting the used fee from the total fee budget.
+ route_params.max_total_routing_fee_msat = route_params
+ .max_total_routing_fee_msat.map(|m| m.saturating_sub(total_ok_fees_msat));
+ route_params.final_value_msat = pending_amt_unsent;


Not a regression, but this is wrong - this includes any overpayment amount, ensuring we overpay again when we go to retry, rather than just recalculating the amount sent against the intended amount. Lets definitely fix this ASAP.

(nevermind my previous reply): Tacked a corresponding commit onto #2575.

Actually, thinking about this once more, I think my previous comment was kinda correct: if we find a route and need to overpay for some of the paths, we may reduce the value of the others to meet the final value, i.e., the route's split is essentially determined at this time. If now some of the overpaid paths fail and we start to only retry the net. value (excluding overpaid amounts), the sum of all paths might not meet the expected final value. I suspect this is no issue if we have infinite auto-retries as we'd retry until the expected value is met, but if we're limited we probably will run out of retry attempts sooner than expected/necessary. Not sure how we'd want to resolve this?

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).

My point here is we don't need to think about paths in this specific case - we tried to send a payment for route.route_params.final_value_msat and we now have route.get_total_amount() - pending_amt_unsent pending. We should retry pending_amt_unsent - (route.get_total_amount() - route.route_params.final_value_msat).
Right, then we seem to be on the same page that per-path accounting doesn't make sense, but only to account for the overpayment for the whole route.
I think what you propose unfortunately doesn't work as is though as for the immediate/legacy retry case we'd recursively reduce final_value_msat while we don't do this in the newer auto-retry case where we'd leave final_value_msat static, but account for for the pending amounts differently. In d7ab0ef I solved this by taking the min, but this seems to be pretty hacky.

TheBlueMatt · 2023-09-26T20:06:35Z

 }

+#[test]
+fn mpp_retry_overpay() {


Hmm, this is nice but isn't what I was mostly interested about. I'm really curious to test overpaying the final recipient and ensuring the route_params' final_value_msat is correct, not just the remaining fees.

Ah, had understood above request slightly differently. IIUC, this might now more or less be covered by the new test introduced in #2604?

That test tests the router end of it, but I'm really worried about the outbound_payment part overpaying.

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

Various router fixes and #2417 follow-ups

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

In lightningdevkit#2417 support for setting a max fee was added but it is not exposed when using the `pay_invoice` utility functions. This makes it so the user can pass in that parameter. One downside is that this removes the defualt value form before.

tnull marked this pull request as draft July 14, 2023 15:08

TheBlueMatt reviewed Jul 14, 2023
View reviewed changes

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

valentinewallace reviewed Jul 14, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt added this to the 0.0.116 milestone Jul 14, 2023

TheBlueMatt assigned TheBlueMatt and valentinewallace Jul 14, 2023

alecchendev reviewed Jul 14, 2023
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated

TheBlueMatt mentioned this pull request Jul 17, 2023
Drop tokio/macros dependency in lightning-net-tokio, fix MSRV #2426
Merged

tnull force-pushed the 2023-07-max-total-fee branch 3 times, most recently from ed0ade8 to da559d3Compare July 18, 2023 12:28

tnull marked this pull request as ready for review July 18, 2023 12:42

tnull force-pushed the 2023-07-max-total-fee branch 3 times, most recently from c34a4a9 to be27e7fCompare July 18, 2023 14:07

tnull commented Jul 18, 2023
View reviewed changes

Comment threadlightning/src/ln/outbound_payment.rs

valentinewallace reviewed Jul 18, 2023
View reviewed changes

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

tnull force-pushed the 2023-07-max-total-fee branch from eaa9435 to 63ed69bCompare July 18, 2023 14:36

valentinewallace reviewed Jul 18, 2023
View reviewed changes

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

tnull force-pushed the 2023-07-max-total-fee branch from 63ed69b to 804abe6Compare July 18, 2023 17:20

tnull force-pushed the 2023-07-max-total-fee branch from 804abe6 to 97f07fbCompare July 18, 2023 20:41

TheBlueMatt reviewed Jul 19, 2023
View reviewed changes

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

tnull force-pushed the 2023-07-max-total-fee branch from 97f07fb to 90ef352Compare July 20, 2023 13:46

tnull removed this from the 0.0.116 milestone Jul 21, 2023

tnull mentioned this pull request Jul 21, 2023
Improve router logging and update documentation #2436
Merged

valentinewallace approved these changes Sep 26, 2023
View reviewed changes

TheBlueMatt approved these changes Sep 26, 2023
View reviewed changes

TheBlueMatt merged commit 1ac53ed into lightningdevkit:mainSep 26, 2023

TheBlueMatt mentioned this pull request Sep 26, 2023
#2417 followups #2600
Closed

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Make ignored candidate counts u32
4b3eabc
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
4478fbd
.. as a follow-up from lightningdevkit#2417.

tnull mentioned this pull request Sep 27, 2023
Various router fixes and #2417 follow-ups #2575
Merged

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Make ignored candidate counts u32
789dfaa
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
071b3f3
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Make ignored candidate counts u32
df4d464
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
85d8341
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Make ignored candidate counts u32
681c0d4
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 27, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
8acbbff
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Make ignored candidate counts u32
2f30ace
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
e985aab
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Make ignored candidate counts u32
9b17172
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
6ae1855
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Make ignored candidate counts u32
f590304
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
d16538b
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Make ignored candidate counts u32
39c5bbc
.. as a follow-up from lightningdevkit#2417.

tnull added a commit to tnull/rust-lightning that referenced this pull request Sep 28, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
fb2c959
.. as a follow-up from lightningdevkit#2417.

TheBlueMatt pushed a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 28, 2023
Make ignored candidate counts u32
2a30a97
.. as a follow-up from lightningdevkit#2417.

TheBlueMatt pushed a commit to TheBlueMatt/rust-lightning that referenced this pull request Sep 28, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
4459588
.. as a follow-up from lightningdevkit#2417.

TheBlueMatt added a commit that referenced this pull request Sep 28, 2023
Merge pull request#2575from tnull/2023-09-fix-debug-panic
1e6707d
Various router fixes and #2417 follow-ups

slanesuke pushed a commit to slanesuke/rust-lightning that referenced this pull request Oct 6, 2023
Make ignored candidate counts u32
119a58e
.. as a follow-up from lightningdevkit#2417.

slanesuke pushed a commit to slanesuke/rust-lightning that referenced this pull request Oct 6, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
2b8c68e
.. as a follow-up from lightningdevkit#2417.

optout21 pushed a commit to optout21/rust-lightning that referenced this pull request Nov 4, 2023
Make ignored candidate counts u32
e313664
.. as a follow-up from lightningdevkit#2417.

optout21 pushed a commit to optout21/rust-lightning that referenced this pull request Nov 4, 2023
Refactor mpp_retry[_overpay] tests to use SendEvent::from_event
58969d1
.. as a follow-up from lightningdevkit#2417.

benthecarman mentioned this pull request Nov 8, 2023
Add max_total_routing_fee_msat for pay_invoice #2719
Closed