Skip to content

Calculate InFlightHtlcs based on information in ChannelManager #1830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Nov 21, 2022
Merged

Calculate InFlightHtlcs based on information in ChannelManager #1830

merged 5 commits into from
Nov 21, 2022

Conversation

jurvis
Copy link
Contributor

@jurvis jurvis commented Nov 4, 2022

Follow-up of #1643 (comment).

Currently, we populate path information about payments we attempt to send/retry in InvoicePayer's payment_cache. It allows us to compute an InFlightHtlcs struct that we pass to the router so that its values can be considered when routing.

This PR introduces an improvement since ChannelManager actually knows about changing liquidity information since it produces the payment events that InvoicePayer relies on to populate path information in payment_cache

@jurvis
Copy link
Contributor Author

jurvis commented Nov 4, 2022

we probably need to rework the testing strategy for InFlightHtlcs' values, but wanted to get a concept ack before proceeding. cc // @jkczyz

@jkczyz jkczyz self-requested a review November 4, 2022 20:17
@jkczyz
Copy link
Contributor

jkczyz commented Nov 4, 2022

Concept ACK with caveats discussed offline about testing and payment cache.

TheBlueMatt
TheBlueMatt reviewed Nov 8, 2022
View reviewed changes
valentinewallace
valentinewallace reviewed Nov 9, 2022
View reviewed changes
@valentinewallace valentinewallace mentioned this pull request Nov 9, 2022
Merged
@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch 2 times, most recently from d5c8465 to 30d7805 Compare November 13, 2022 01:43
jurvis
jurvis commented Nov 13, 2022
View reviewed changes
lightning-invoice/src/payment.rs
@@ -1729,66 +1589,6 @@ mod tests {
});
}

#[test]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer populate inflight path information based on events, so there is probably no need to have tests for this any longer.

@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 30d7805 to 2806be6 Compare November 13, 2022 01:51
@jurvis
Copy link
Contributor Author

jurvis commented Nov 13, 2022

rebased to resolve conflict in payment.rs

@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2022 •
edited
Loading

Codecov Report

Base: 90.61% // Head: 90.97% // Increases project coverage by +0.36% 🎉

Coverage data is based on head (3136d73) compared to base (7269fa2).
Patch coverage: 99.36% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1830      +/-   ##
==========================================
+ Coverage   90.61%   90.97%   +0.36%     
==========================================
  Files          90       91       +1     
  Lines       47623    50202    +2579     
  Branches    47623    50202    +2579     
==========================================
+ Hits        43152    45670    +2518     
- Misses       4471     4532      +61
Impacted Files Coverage Δ
lightning/src/ln/channel.rs 92.13% <90.90%> (+3.38%) ⬆️
lightning-invoice/src/payment.rs 89.27% <100.00%> (-0.45%) ⬇️
lightning-invoice/src/utils.rs 95.20% <100.00%> (+<0.01%) ⬆️
lightning/src/ln/channelmanager.rs 85.11% <100.00%> (+0.07%) ⬆️
lightning/src/ln/payment_tests.rs 98.89% <100.00%> (+0.12%) ⬆️
lightning/src/routing/router.rs 91.73% <100.00%> (+0.03%) ⬆️
lightning/src/offers/offer.rs 92.26% <0.00%> (-2.30%) ⬇️
lightning/src/util/events.rs 25.25% <0.00%> (-0.25%) ⬇️
lightning/src/ln/functional_tests.rs 97.13% <0.00%> (-0.03%) ⬇️
lightning/src/offers/parse.rs 93.47% <0.00%> (ø)
... 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.

@jurvis jurvis marked this pull request as ready for review November 13, 2022 04:17
valentinewallace
valentinewallace reviewed Nov 14, 2022
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@jurvis
Copy link
Contributor Author

jurvis commented Nov 14, 2022

thanks for the review @valentinewallace! I'll make the changes.

additional note: I'm going to be writing an additional tests to make sure HTLCs in holding cells are calculated when producing InflightHtlcs.

@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 853282a to 673d36f Compare November 15, 2022 05:03
@jurvis
Copy link
Contributor Author

jurvis commented Nov 15, 2022

Resolved review comments, added a functional test in 673d36f to check if holding cells HTLCs show up in InflightHtlcs

valentinewallace
valentinewallace reviewed Nov 15, 2022
View reviewed changes
lightning/src/ln/channelmanager.rs Outdated
Comment on lines 5706 to 5707
/// Gets inflight HTLC information by processing pending outbound `HTLCSource`s that are in
/// our channels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should document the usage, because IIRC this isn't intended to be used for users to view their in-flight payments, just for routing purposes? Lmk if I'm off there

Copy link
Contributor Author

@jurvis jurvis Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's correct. We'll work on an API for viewing pending payments in a follow-up PR, since we think it may need a different solution.

I'll add a little bit more context here to make its intended use more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentinewallace I added a comment to specify that we use this information for pathfinding, but let me know if it can be better :)

TheBlueMatt
TheBlueMatt reviewed Nov 15, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, otherwise.

@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 673d36f to 2c8fdd3 Compare November 16, 2022 02:56
TheBlueMatt
TheBlueMatt previously approved these changes Nov 16, 2022
View reviewed changes
jkczyz
jkczyz reviewed Nov 16, 2022
View reviewed changes
@TheBlueMatt TheBlueMatt mentioned this pull request Nov 16, 2022
Merged
@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 2c8fdd3 to 8449c71 Compare November 17, 2022 00:54
@valentinewallace
Copy link
Contributor

Needs rebase as well, sorry!

@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 8449c71 to 7d6b109 Compare November 17, 2022 01:50
@jurvis
Copy link
Contributor Author

jurvis commented Nov 17, 2022

@valentinewallace thanks for pointing that out -- rebased!

valentinewallace
valentinewallace reviewed Nov 17, 2022
View reviewed changes
@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 7d6b109 to b51db04 Compare November 17, 2022 17:47
@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from b51db04 to 3e8926d Compare November 17, 2022 18:50
valentinewallace
valentinewallace previously approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, feel free to take or leave nits

@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from 3e8926d to fd4792a Compare November 17, 2022 20:18
@jurvis
Copy link
Contributor Author

jurvis commented Nov 17, 2022

@valentinewallace nits very much appreciated :) thanks!

TheBlueMatt
TheBlueMatt previously approved these changes Nov 17, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will let @jkczyz review + merge.

jkczyz
jkczyz reviewed Nov 18, 2022
View reviewed changes
@jurvis jurvis mentioned this pull request Nov 18, 2022
Closed
@jurvis
Compute InflightHtlcs from available information in ChannelManager
89f162c
@jurvis
Remove paths from PaymentInfo in payment_cache
2e81813 
In c70bd1f, we implemented tracking HTLCs by adding path information
for pending HTLCs to `InvoicePayer`’s `payment_cache` when receiving
specific events.

Since we can now track inflight HTLCs entirely within ChannelManager,
there is no longer a need for this to exist.
@jurvis
Add functional test for inflight HTLC tracking with ChanManager
84ba180
@jurvis
Remove pub visibility of InFlightHtlcs HashMap
3136d73
@jurvis jurvis dismissed stale reviews from TheBlueMatt and valentinewallace via 3136d73 November 19, 2022 19:20
@jurvis jurvis force-pushed the jurvis/2022-10-calculate-inflight-with-chanmanager branch from fd4792a to 3136d73 Compare November 19, 2022 19:20
@TheBlueMatt TheBlueMatt merged commit a4c4301 into lightningdevkit:main Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@jkczyz jkczyz jkczyz approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

Assignees

@TheBlueMatt TheBlueMatt

@jkczyz jkczyz

@valentinewallace valentinewallace

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jurvis @jkczyz @codecov-commenter @valentinewallace @TheBlueMatt