Prepare for Payment Retries in `ChannelManager` #1862

valentinewallace · 2022-11-18T19:13:52Z

Just code moves that help us prepare for getting rid of the InvoicePayer and moving payment retries into ChannelManager, in preparation for trampoline payments.

lightning/src/routing/router.rs

valentinewallace · 2022-11-18T19:26:05Z

After we sort out whether to make ScorerAccountingForInFlightHtlcs public I can update its methods and docs.

codecov-commenter · 2022-11-18T19:32:28Z

Codecov Report

Base: 90.72% // Head: 91.02% // Increases project coverage by +0.30% 🎉

Coverage data is based on head (70ca0d4) compared to base (8245128).
Patch coverage: 37.50% of modified lines in pull request are covered.

❗ Current head 70ca0d4 differs from pull request most recent head 3f53951. Consider uploading reports for the commit 3f53951 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1862      +/-   ##
==========================================
+ Coverage   90.72%   91.02%   +0.30%     
==========================================
  Files          91       91              
  Lines       47896    49984    +2088     
  Branches    47896    49984    +2088     
==========================================
+ Hits        43453    45499    +2046     
- Misses       4443     4485      +42

Impacted Files	Coverage Δ
lightning-background-processor/src/lib.rs	`96.40% <ø> (+0.96%)`	⬆️
lightning-invoice/src/payment.rs	`89.48% <ø> (+0.21%)`	⬆️
lightning-invoice/src/utils.rs	`97.81% <ø> (+2.60%)`	⬆️
lightning/src/routing/router.rs	`90.86% <13.79%> (-0.87%)`	⬇️
lightning/src/routing/scoring.rs	`96.92% <100.00%> (+0.03%)`	⬆️
lightning-net-tokio/src/lib.rs	`76.73% <0.00%> (-0.31%)`	⬇️
lightning/src/ln/peer_channel_encryptor.rs	`93.38% <0.00%> (-0.25%)`	⬇️
lightning/src/ln/monitor_tests.rs	`99.44% <0.00%> (-0.12%)`	⬇️
lightning/src/ln/functional_tests.rs	`97.07% <0.00%> (-0.04%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.31% <0.00%> (-0.03%)`	⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt · 2022-11-22T21:23:17Z

Hmm, is there some way to retain the find_route_with_id logic here? Even if we move retries to ChannelManager we still want to pass the PaymentId into the router, I think.

valentinewallace · 2022-11-22T22:12:41Z

Hmm, is there some way to retain the find_route_with_id logic here? Even if we move retries to ChannelManager we still want to pass the PaymentId into the router, I think.

For sure, how about an additional Router method with a default implementation?

TheBlueMatt · 2022-11-23T01:23:04Z

That seems reasonable to me.

lightning/src/routing/scoring.rs

TheBlueMatt · 2022-11-29T17:50:40Z

Feel free to squash, IMO.

This helps us prepare to move all payment retries into ChannelManager, which is needed for trampoline payments.

We move it to router instead of scoring because it pairs with the InFlightHtlcs struct in router and is useful for custom Router trait implementations

jkczyz · 2022-11-30T23:10:25Z

lightning-invoice/src/payment.rs

 	use bitcoin_hashes::sha256::Hash as Sha256;
 	use lightning::ln::PaymentPreimage;
 	use lightning::ln::channelmanager;
 	use lightning::ln::features::{ChannelFeatures, NodeFeatures};
 	use lightning::ln::functional_test_utils::*;
 	use lightning::ln::msgs::{ChannelMessageHandler, ErrorAction, LightningError};
 	use lightning::routing::gossip::{EffectiveCapacity, NodeId};
-	use lightning::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, Router};
+	use lightning::routing::router::{InFlightHtlcs, PaymentParameters, Route, RouteHop, Router, ScorerAccountingForInFlightHtlcs};


It's a little strange that we test ScorerAccountingForInFlightHtlcs in this crate still in considers_inflight_htlcs_between_retries. Probably should move the test.

Could we save that since all of the tests are going to be moving soon?

valentinewallace commented Nov 18, 2022

View reviewed changes

lightning/src/routing/router.rs Show resolved Hide resolved

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch 2 times, most recently from 94fcf48 to ea0c8cb Compare November 21, 2022 23:35

valentinewallace marked this pull request as ready for review November 22, 2022 00:02

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch 5 times, most recently from e38cd12 to 59510b9 Compare November 22, 2022 16:12

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch from 59510b9 to 45cca60 Compare November 23, 2022 17:49

jkczyz reviewed Nov 23, 2022

View reviewed changes

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

lightning/src/routing/scoring.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch from 45cca60 to 70ca0d4 Compare November 23, 2022 22:41

valentinewallace added the Seeking Code Review label Nov 23, 2022

TheBlueMatt assigned TheBlueMatt and jkczyz Nov 29, 2022

TheBlueMatt removed the Seeking Code Review label Nov 29, 2022

valentinewallace added 2 commits November 29, 2022 12:52

Move ScoringRouter methods to Router

685b370

This helps us prepare to move all payment retries into ChannelManager, which is needed for trampoline payments.

Fix typo in ScorerAccountingForInFlightHtlcs

5f49011

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch from 70ca0d4 to 3f53951 Compare November 29, 2022 17:52

TheBlueMatt previously approved these changes Nov 29, 2022

View reviewed changes

valentinewallace mentioned this pull request Nov 29, 2022

Parameterize ChannelManager by a Router #1812

Merged

valentinewallace added 2 commits November 30, 2022 16:20

Move ScorerAccountingForInFlightHtlcs to router + make public

3f9868f

We move it to router instead of scoring because it pairs with the InFlightHtlcs struct in router and is useful for custom Router trait implementations

Move DefaultRouter to router module

8a51a79

valentinewallace dismissed TheBlueMatt’s stale review via 8a51a79 November 30, 2022 21:30

valentinewallace force-pushed the 2022-11-chanman-retries-prep branch from 3f53951 to 8a51a79 Compare November 30, 2022 21:30

jkczyz reviewed Nov 30, 2022

View reviewed changes

jkczyz approved these changes Dec 1, 2022

View reviewed changes

TheBlueMatt approved these changes Dec 1, 2022

View reviewed changes

TheBlueMatt merged commit 5e577cb into lightningdevkit:main Dec 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare for Payment Retries in `ChannelManager` #1862

Prepare for Payment Retries in `ChannelManager` #1862

valentinewallace commented Nov 18, 2022

valentinewallace commented Nov 18, 2022

codecov-commenter commented Nov 18, 2022 •

edited

Loading

TheBlueMatt commented Nov 22, 2022

valentinewallace commented Nov 22, 2022

TheBlueMatt commented Nov 23, 2022

TheBlueMatt commented Nov 29, 2022

jkczyz Nov 30, 2022

valentinewallace Nov 30, 2022

Prepare for Payment Retries in ChannelManager #1862

Prepare for Payment Retries in ChannelManager #1862

Conversation

valentinewallace commented Nov 18, 2022

valentinewallace commented Nov 18, 2022

codecov-commenter commented Nov 18, 2022 • edited Loading

Codecov Report

TheBlueMatt commented Nov 22, 2022

valentinewallace commented Nov 22, 2022

TheBlueMatt commented Nov 23, 2022

TheBlueMatt commented Nov 29, 2022

jkczyz Nov 30, 2022

Choose a reason for hiding this comment

valentinewallace Nov 30, 2022

Choose a reason for hiding this comment

Prepare for Payment Retries in `ChannelManager` #1862

Prepare for Payment Retries in `ChannelManager` #1862

codecov-commenter commented Nov 18, 2022 •

edited

Loading