Skip to content
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.

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

Renepay: htlc limits on mcf #6905

Merged
merged 2 commits into from Feb 2, 2024

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Nov 29, 2023

Usually minflow would first compute a MCF solution and then construct routes based on that.
Neither of these steps would take into account the limits htlc_min and htlc_max on channels.

PR #6893 removes a portion of the amount committed to the routes if they exceed the htlc_max,
but that PR assumes that that excess is small and it is due to the MCF not taking into account the
addition of fees to the flow. If the amount committed to a route exceeds the htlc_max in a great
amount, we would be better off to allocate that difference in other routes by recomputing MCF.
But we don't have a check for the size of that number and we just assume that the excess is small.

I think a better way to constraint the flow with respect to the htlc_max (and htlc_min) bounds is
to do it at a lower level: either when computing MCF or when dissecting the solution into routes.
The easiest solution is the latter and this is what we do in the current PR.

Once the MCF is solved, we start constructing flow routes. On those routes we can compute the
smallest of the htlc_max and the biggest of the htlc_min and use those as limits to the amount
we can send. When these flow routes are passed to flows_fit_amount we will be assured that any
amount exceeding htlc_max would be because of the addition of fees.

  • implementation
  • write tests

@Lagrang3 Lagrang3 marked this pull request as draft November 29, 2023 11:09
@Lagrang3 Lagrang3 marked this pull request as ready for review November 30, 2023 11:45
@renepickhardt
Copy link
Collaborator

Thanks for prioritizing this! I think this will be a very important issue.

Without having looked at the code I wonder what happens if the solution of a flow wants to allocate 10M sats on a channel (of a higher capacity) but the channel has for example an htlc_maximum_msat of 500k sats but only allows 10 concurrent inflight HTLCs? In this way I assume the dissection would create 20 onions of which only the first 10 would be accepted by the channel.

Thus I think it makes sense flow computation (before linearization) to set the capacity of channels at most to: min(channel_capacity_from_funding_tx, htlc_maximum_msat * max_concurrent_htlcs)

On the long term I think for over all network reliability it makes sense to

  1. use htlc_maximum_msat as the capacity in the uncertainty network
  2. use the amount c of the funding transaction outpoint within the cost function to produce the probabilistic prior

I do understand that my proposal may tend to avoid channels for a second time even if that channel could potentially forward two concurrent htlcs. However I think that kind of default behavior has overall better properties for the semantics of htlc_maximum_msat (c.f.: https://blog.bitmex.com/the-power-of-htlc_maximum_msat-as-a-control-valve-for-better-flow-control-improved-reliability-and-lower-expected-payment-failure-rates-on-the-lightning-network/ )

@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Dec 5, 2023

Without having looked at the code I wonder what happens if the solution of a flow wants to allocate 10M sats on a channel (of a higher capacity) but the channel has for example an htlc_maximum_msat of 500k sats but only allows 10 concurrent inflight HTLCs? In this way I assume the dissection would create 20 onions of which only the first 10 would be accepted by the channel.

yes

Thus I think it makes sense flow computation (before linearization) to set the capacity of channels at most to: min(channel_capacity_from_funding_tx, htlc_maximum_msat * max_concurrent_htlcs)

Agree.

use htlc_maximum_msat as the capacity in the uncertainty network

We might allow for a second and a third HTLC on the same channel by using min(channel_capacity_from_funding_tx, htlc_maximum_msat * 3) as the arc capacity.

@renepickhardt
Copy link
Collaborator

The magic number 3 seems somewhat arbitrary.

However one novel thought just came to my mind: One could choose the arcs in the piecewise linearization to have a capacity of htlc_maximum_msat. The increasing cost of the arcs in flow computation would also somehow reflect the multiple payments of the basefee in case several arcs are being used. The number of arcs could then be chosen to reflect both the channel's capacity as well as max_concurrent_htlcs.

Downsides: Even if a single arc is being used the disection of flow into onions could require to still have more than 1 htlc on the channel. Best way to mitigate such problems if in future the protocol would be more suitable to the flow like nature of payments. Eg including update_update_htlc allowing peers to change the amount of an already inflight htlcs.
Also the number of arcs is currently pretty static in the code base.

@Lagrang3 Lagrang3 removed the request for review from rustyrussell December 6, 2023 18:12
@Lagrang3 Lagrang3 marked this pull request as draft December 6, 2023 18:13
@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Dec 6, 2023

I see the case for using htlc_maximum_msat as a constraint to the amount of flow we want to send through a channel,
because we don't want to spam remote channels with many HTLCs. This however does not prevent the MCF algorithm to find a solution in which distinct routes reuse the same channel, the constraint only says "the sum of all flows on that channel cannot exceed htlc_maximum_msat", just saying.

Also, would it make sense to apply the same constraint to local channels?

@renepickhardt
Copy link
Collaborator

Sorry for the meta comment: I see the problems that of course a remote channel could still have more than 1 HTLCs. Also not sure about local channels. I tend to say we could be a bit more relaxed about those as we don't pay base fee and have no uncertainty and actually are wiling to use our full liquidity. I was wondering weather this could be configurable. In this way we would not decide for the user. However I assume it would be very hard to create an API that users understand. Having parameters that people do not understand intuitively how to use is always a bit frustrating.

@vincenzopalazzo vincenzopalazzo self-assigned this Dec 7, 2023
@vincenzopalazzo vincenzopalazzo added this to the v24.02 milestone Dec 7, 2023
@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Dec 7, 2023

There are 3 places in the code where we can enforce that the payment routes (aka flows) respect the htlc_maximum limit they are:

  1. In flow.c function flow_complete that assigns the actual route amounts to the flows proposed by minflow. We do this in PR renepay: accomodate fees in the payment flow #6893. But this solution is weak in the sense that it works only after minflow as computed the routes, so it tries to fix the missing payment amounts without optimizing costs.

  2. In mcf.c function get_flow_paths that transforms the solution of the MCF algorithm into a series of routes. In this PR we try to cap the flows there. Basically this is like saying: "get me the solution of the MCF and I will construct payment routes such that each route has an amount at every channel that do not exceed the htlc_max".

  3. In mcf.c function linearize_channel in which we can put a capacities (in the MCF sense) to the channel arcs that do not exceed hltc_maximum. "I will compute a MCF solution such that the sum of all htlcs on every channel do not exceed hltc_max". It is a stronger condition than 2, if you think of it, 3 implies 2, but 2 does not imply 3. Also this alternative reduces the number of HTLCs employed, but it does it at the expenses of our reliability to fulfill the payment.

I would say, since point 1 is already being pushed in PR #6893, let's keep this PR as the implementation of point 2.
And open a third PR for point 3.

@Lagrang3 Lagrang3 marked this pull request as ready for review December 7, 2023 10:33
@cdecker
Copy link
Member

cdecker commented Jan 15, 2024

Between this and #6893, which one do you think solves this issue best @renepickhardt and @Lagrang3 ? It is marked for the 24.02 milestone, and I'd like to merge one of the two solutions before the feature freeze.

@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Jan 16, 2024

Hi @cdecker. Short answer: both are needed. They do different things, but they also happen to overlap on checking that the HTLC are within the given bounds.

The current problems we want to solve in renepay are the following:
A. when routes are constructed the htlc_max and htlc_min are not taken into account,
B. fees cannot be precisely computed in the MCF, hence they are added after the routes are constructed and hence it can happen that transport+fees exceed the knowledge bounds.

#6893 was meant to solve B.
It also happens that #6893 also solves A, but it does at the expense of extra MCF computations. This is because the amount that cannot fit the constraints are allocated either into already proposed routes or if they cannot fit, another MCF computation is used only for this excess amount. The probability of needing to call MCF again is smaller if HTLC limits are already satisfied.

This is where #6905 comes in.
It ensures that HTLC limits are respected when routes are constructed. In this way down the pipeline the logic introduced in #6893 will have an easier task to solve needing only to make fees fit in the flows.

@Lagrang3
Copy link
Collaborator Author

The discussion with @renepickhardt was mainly because instead of limiting the amount during the route construction phase, we could do it at the MCF level. But that is not implemented neither here nor in #6893. This would be a consideration for a future PR.

@Lagrang3 Lagrang3 force-pushed the renepay_htlc_limits_on_mcf branch 2 times, most recently from 9982677 to 0807bff Compare January 29, 2024 10:01
@Lagrang3
Copy link
Collaborator Author

Rebased and fixed conflicts.

@cdecker
Copy link
Member

cdecker commented Feb 2, 2024

Great, thanks for the explanation, that makes sense. If that's ok with you, I can shepherd this PR through CI, and get it merged asap :-)

    The flow routes returned by minflow are bounded by the htlc_min and
    htlc_max along the route whenever possible.
@cdecker
Copy link
Member

cdecker commented Feb 2, 2024

Rebased on top of master

@cdecker cdecker enabled auto-merge (rebase) February 2, 2024 09:36
@cdecker cdecker merged commit 59935c5 into ElementsProject:master Feb 2, 2024
32 of 35 checks passed
@Lagrang3 Lagrang3 deleted the renepay_htlc_limits_on_mcf branch February 2, 2024 12:57
@Lagrang3 Lagrang3 mentioned this pull request Mar 27, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants