Move `InflightHtlcs` and `Router` trait into `ChannelManager` #1811

valentinewallace · 2022-10-27T22:13:02Z

This prepares us to parameterize ChannelManager with a Router trait, which will then be used to fetch routes on-the-fly for trampoline payments.

Relates to #1157 although trampoline is not blocked by it -- ChannelManager can just not factor in in-flight HTLCs for now.

This helps prepare for moving the Router trait into ChannelManager, which will help allow ChannelManager to retrieve routes for trampoline

codecov-commenter · 2022-10-27T22:44:45Z

Codecov Report

Base: 90.72% // Head: 91.41% // Increases project coverage by +0.69% 🎉

Coverage data is based on head (7db8052) compared to base (6957fb6).
Patch coverage: 69.23% of modified lines in pull request are covered.

❗ Current head 7db8052 differs from pull request most recent head df593dc. Consider uploading reports for the commit df593dc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1811      +/-   ##
==========================================
+ Coverage   90.72%   91.41%   +0.69%     
==========================================
  Files          87       87              
  Lines       47361    51151    +3790     
  Branches    47361    51151    +3790     
==========================================
+ Hits        42968    46761    +3793     
+ Misses       4393     4390       -3

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	`95.17% <ø> (ø)`
lightning/src/routing/router.rs	`93.18% <ø> (+1.56%)`	⬆️
lightning-invoice/src/payment.rs	`89.68% <50.00%> (-0.17%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.19% <85.71%> (+<0.01%)`	⬆️
lightning/src/ln/monitor_tests.rs	`99.55% <0.00%> (+0.11%)`	⬆️
lightning/src/ln/peer_channel_encryptor.rs	`93.62% <0.00%> (+0.24%)`	⬆️
lightning/src/util/events.rs	`38.40% <0.00%> (+0.26%)`	⬆️
lightning/src/chain/onchaintx.rs	`95.67% <0.00%> (+0.38%)`	⬆️
lightning/src/util/test_utils.rs	`77.06% <0.00%> (+0.66%)`	⬆️
lightning/src/onion_message/functional_tests.rs	`96.25% <0.00%> (+0.76%)`	⬆️
... and 7 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.

jkczyz · 2022-10-28T15:51:29Z

lightning-invoice/src/payment.rs

-		&self, payer: &PublicKey, route_params: &RouteParameters, payment_hash: &PaymentHash,
+		&self, payer: &PublicKey, route_params: &RouteParameters,


Not sure if we want to do this. It's for implementations wanting to support looking up pre-computed routes from a probe, according to the commit when added.

592bfd7

It wouldn't be ideal to provide users with the payment hash in trampoline payments.

Do either of these options seem reasonable:
(1) make payment_hash into an Option<(PaymentHash, PaymentId)>
(2) make payment_hash into an Option<PaymentId>

It wouldn't be ideal to provide users with the payment hash in trampoline payments.

Could you expand on this? Is the hash not available? Or is providing it harmful / misleading in some way?

I think it's leaking the sender's privacy since it's not our payment

We also can't provide a PaymentId for trampoline payments, which we'd like to, I think. We might also want to keep a separate routing trait for the payer? Ugh...

It wouldn't be ideal to provide users with the payment hash in trampoline payments.

Do either of these options seem reasonable: (1) make payment_hash into an Option<(PaymentHash, PaymentId)> (2) make payment_hash into an Option<PaymentId>

Where is PaymentId coming from / why is it needed?

I think it's leaking the sender's privacy since it's not our payment

Not really sure I follow this. If our node knows the payment hash, hasn't it already been leaked?

IIUC there's still a difference in privacy between seeing it in an internal update_add_htlc message vs actually providing it to a user in a trait method.

Where is PaymentId coming from / why is it needed?

Same reason a payment_hash is being provided in 592bfd7 I think, to have another option to look up pre-computed probes. As of #1761, PaymentIds will be required upon all payment initiations, so it'd come from there for non-trampoline find_route calls.

lightning-invoice/src/payment.rs

jkczyz · 2022-10-28T16:02:02Z

lightning/src/ln/channelmanager.rs

+/// A map with liquidity value (in msat) keyed by a short channel id and the direction the HTLC
+/// is traveling in. The direction boolean is determined by checking if the HTLC source's public
+/// key is less than its destination. See [`InFlightHtlcs::used_liquidity_msat`] for more
+/// details.
+#[cfg(not(any(test, feature = "_test_utils")))]
+pub struct InFlightHtlcs(HashMap<(u64, bool), u64>);
+#[cfg(any(test, feature = "_test_utils"))]
+pub struct InFlightHtlcs(pub HashMap<(u64, bool), u64>);
+
+impl InFlightHtlcs {
+	/// Create a new `InFlightHtlcs` via a mapping from:
+	/// (short_channel_id, source_pubkey < target_pubkey) -> used_liquidity_msat
+	pub fn new(inflight_map: HashMap<(u64, bool), u64>) -> Self {
+		InFlightHtlcs(inflight_map)
+	}


Maybe could be left to a later refactor (cc: @jurvis), but it seems odd we expose the implementation details about the HashMap. Perhaps we should have a method for adding paths.

Another thought -- when adding this to the Payer interface -- is to have an associated type with a trait bound on some new trait having a used_liquidity_msat. Though that would mean Router and Payer would need to agree on that associated type as it would be need in Router, too.

It does seem like the best way here could depend on how #1157 shakes out. I agree it's weird but I'm not 100% sure the best approach that wouldn't churn. Open to @jurvis's and your thoughts though if there seems to be a clear way.

when adding this to the Payer interface

Not following, what is being added to Payer?

Not following, what is being added to Payer?

A method for accessing InFlightHtlcs. Background: #1643 (comment)

Yeah, I wouldn't be opposed to an associated type. I'm reluctant to be totally blocked on that issue though. Do you think there's something that can be done to make progress in the meantime here?

I don't really understand why an associated type makes sense - what implementation other than a HashMap would someone want to use for this?

Was leaning towards the former approach. Anyhow, this is orthogonal to the PR. Figured @jurvis would tackle it when updating the Payer trait.

jkczyz · 2022-10-28T16:04:03Z

lightning/src/ln/channelmanager.rs

+	/// Lets the router know that payment through a specific path has failed.
+	fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64);
+	/// Lets the router know that payment through a specific path was successful.
+	fn notify_payment_path_successful(&self, path: &[&RouteHop]);


Will ChannelManager need to care about these methods? Seems like the distinction isn't probing vs non-probing but rather routing vs event handling.

ChannelManager as an async sending LSP has an opportunity to collect a lot of useful payment success/failure data for scoring. That said, I'm ok leaving these methods out for now if it's preferred

Hmm... so essentially ChannelManager wouldn't surface any events for the user to handle but would handle them internally. Wouldn't it also need to retry failures? Seems like something more akin to InvoicePayer would be required rather than just the Router.

Discussed offline, yes that's accurate but we're not sure what to do yet. W/r/t to the question at hand, would you prefer to remove these methods for now?

Discussed offline, yes that's accurate but we're not sure what to do yet. W/r/t to the question at hand, would you prefer to remove these methods for now?

Yeah, let's remove them... But that said, should we even bother with the interface churn now -- given this is for experimentation -- if it will likely to be different when accounting for retires? That is, can't the experiment simply call the standalone find_route function directly for now?

Right, i'm definitely gonna remove the payment_path_* methods for now. I was hoping to get your input on:

should we even bother with the interface churn now -- given this is for experimentation -- if it will likely to be different when accounting for retires? That is, can't the experiment simply call the standalone find_route function directly for now?

@TheBlueMatt

I don't see why that changes anything? Yes, we'll want to provide the "previous path attempts" set to the router just like we do for regular payments, but that can come as a parameter (eg via RouteParameters, just like we do for our own payments).

The concern is about essentially re-writing the retry logic of InvoicePayer here. I'm not yet convinced parameterizing ChannelManager with a Router is the right approach. Even if it is, it sorta begs the question as to why we shouldn't move all of InvoicePayer's retry logic inside ChannelManager rather than using InvoicePayer as an EventHandler wrapper, as we currently do. Just think we need to put some more thought into this design.

Ah, sorry, I thought you meant passing previous-try info into the Router. Right, I'd think we'd want to move the retry logic into the ChannelManager, though that seems independent, I dont think the Router trait would look any different once we move retry logic into ChannelManager, so this would still be a good first step.

Indeed, if we didnt want to move retrying into the ChannelManager then we'd probably have this look very different, but the question there seems to me to be "do we want/expect every routing node to support trampoline, or is it really just an LSP-level thing". If we think its an LSP-level thing I think we shouldn't do trampoline at all (it has terrible privacy in that kind of deployment). If we think it should be a part of every routing node, then pushing the trampoline handling out to an event would force users into a good bit of boilerplate that they really have no choice but to implement, which seems wasteful.

Adding retry logic into ChannelManager also seems kinda nice, it simplifies the amount of boilerplate users have to have for handling their own payments, which we've heard is an issue for users, and its no less expressible - you can always pass a RetryCount(0) when sending a payment and handle it yourself.

Discussed offline, gonna move forward with parameterizing ChannelManager with Router but reassess overall API design at each step. There may be an opportunity to break ChannelManager up into multiple subsystems (e.g. payment forwarding) to clean things up along the way.

@jkczyz can speak more to this, but it may be advantageous to create a Node abstraction over InvoicePayer / ChannelManager (which could be renamed).

jkczyz · 2022-11-01T22:35:42Z

lightning-invoice/src/payment.rs

-		&self, payer: &PublicKey, route_params: &RouteParameters, payment_hash: &PaymentHash,
+		&self, payer: &PublicKey, route_params: &RouteParameters,


It wouldn't be ideal to provide users with the payment hash in trampoline payments.

Do either of these options seem reasonable: (1) make payment_hash into an Option<(PaymentHash, PaymentId)> (2) make payment_hash into an Option<PaymentId>

Where is PaymentId coming from / why is it needed?

I think it's leaking the sender's privacy since it's not our payment

Not really sure I follow this. If our node knows the payment hash, hasn't it already been leaked?

lightning-invoice/src/payment.rs

lightning/src/ln/channelmanager.rs

lightning-invoice/src/payment.rs

TheBlueMatt · 2022-11-01T22:48:48Z

lightning-invoice/src/payment.rs

+/// and scoring based on payment success data.
+///
+/// [`Router`]: lightning::ln::channelmanager::Router
+pub trait ScoringRouter: Router {


Why don't we define a different find_route function here that does include the PaymentId/PaymentHash and just call out to the underlying find_route by default?

Not sure I follow why we want to do this and why it shouldn't be on Router instead. Doesn't look like it's used here?

lightning/src/ln/channelmanager.rs

TheBlueMatt

Basically LGTM.

lightning-invoice/src/payment.rs

jkczyz

Feel free to squash.

lightning/src/routing/router.rs

This is part of moving the Router trait into ChannelManager, which will help allow ChannelManager to fetch routes on-the-fly as part of supporting trampoline payments.

This helps prepare to parameterize ChannelManager with a Router, to eventually use in trampoline payments.

valentinewallace added 2 commits October 27, 2022 18:02

Fix inaccurate comment in InvoicePayer

59e24bd

Remove unused payment_hash param from Router::find_route

dc7f65f

This helps prepare for moving the Router trait into ChannelManager, which will help allow ChannelManager to retrieve routes for trampoline

jkczyz self-requested a review October 28, 2022 15:31

valentinewallace added the Seeking Code Review label Oct 28, 2022

valentinewallace mentioned this pull request Oct 28, 2022

Parameterize ChannelManager by a Router #1812

Merged

jkczyz reviewed Oct 28, 2022

View reviewed changes

valentinewallace force-pushed the 2022-10-chanman-router branch from 8828fce to 8b4fed2 Compare October 28, 2022 20:10

jkczyz reviewed Nov 1, 2022

View reviewed changes

TheBlueMatt reviewed Nov 1, 2022

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-10-chanman-router branch from d054b25 to 8ca062f Compare November 2, 2022 15:18

TheBlueMatt reviewed Nov 2, 2022

View reviewed changes

lightning-invoice/src/payment.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 2022-10-chanman-router branch from 7db8052 to df593dc Compare November 3, 2022 20:01

jkczyz reviewed Nov 3, 2022

View reviewed changes

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

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

valentinewallace added 2 commits November 3, 2022 16:19

Move InFlightHtlcs into ChannelManager

1840cae

This is part of moving the Router trait into ChannelManager, which will help allow ChannelManager to fetch routes on-the-fly as part of supporting trampoline payments.

Move InvoicePayer's Router into ChannelManager

9d33249

This helps prepare to parameterize ChannelManager with a Router, to eventually use in trampoline payments.

valentinewallace force-pushed the 2022-10-chanman-router branch from df593dc to 9d33249 Compare November 3, 2022 20:22

jkczyz approved these changes Nov 3, 2022

View reviewed changes

TheBlueMatt removed the Seeking Code Review label Nov 3, 2022

TheBlueMatt approved these changes Nov 3, 2022

View reviewed changes

TheBlueMatt merged commit e55e0d5 into lightningdevkit:main Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `InflightHtlcs` and `Router` trait into `ChannelManager` #1811

Move `InflightHtlcs` and `Router` trait into `ChannelManager` #1811

valentinewallace commented Oct 27, 2022

codecov-commenter commented Oct 27, 2022 •

edited

Loading

jkczyz Oct 28, 2022

valentinewallace Oct 28, 2022

jkczyz Oct 31, 2022

valentinewallace Oct 31, 2022

TheBlueMatt Oct 31, 2022

jkczyz Nov 1, 2022

valentinewallace Nov 2, 2022

jkczyz Oct 28, 2022

valentinewallace Oct 28, 2022

jkczyz Oct 31, 2022

valentinewallace Oct 31, 2022

TheBlueMatt Oct 31, 2022

jkczyz Oct 31, 2022

jkczyz Oct 28, 2022

valentinewallace Oct 28, 2022

jkczyz Oct 31, 2022

valentinewallace Oct 31, 2022

jkczyz Oct 31, 2022

valentinewallace Oct 31, 2022

TheBlueMatt Oct 31, 2022

jkczyz Oct 31, 2022

TheBlueMatt Oct 31, 2022

valentinewallace Nov 2, 2022

jkczyz Nov 1, 2022

TheBlueMatt Nov 1, 2022

jkczyz Nov 3, 2022

TheBlueMatt left a comment

jkczyz left a comment

		&self, payer: &PublicKey, route_params: &RouteParameters, payment_hash: &PaymentHash,
		&self, payer: &PublicKey, route_params: &RouteParameters,

Move InflightHtlcs and Router trait into ChannelManager #1811

Move InflightHtlcs and Router trait into ChannelManager #1811

Conversation

valentinewallace commented Oct 27, 2022

codecov-commenter commented Oct 27, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

Move `InflightHtlcs` and `Router` trait into `ChannelManager` #1811

Move `InflightHtlcs` and `Router` trait into `ChannelManager` #1811

codecov-commenter commented Oct 27, 2022 •

edited

Loading