MPP: limit the number of HTLCs based on destination connectivity

[ZmnSCPxj]

Fixes: #3926

(probably)

[ZmnSCPxj]

Rebased

[ZmnSCPxj]

Currently wondering if I should instead put the new code in a new paymod instead of mixing it with the presplitter.

And additionally make the presplitter less aggressive, say if it would generate more HTLCs than half the payment_max_htlcs it would limit itself to half the payment_max_htlcs, given that we can very easily split later but have no way to re-merge payments.

[ZmnSCPxj]

Turns out the routehints paymod takes over ownership of the invoice routes (and leaves a stale pointer in the invoice structure, how rude), but we need to know the number of invoice routes being maintained, because if the payee is unpublished that is the only indication we have of the incoming channels of the payee and therefore the limit on the destination connectivity.

[whitslack]

This seems extremely fragile. You're making some wild assumptions about the HTLC capacities of the nodes in the network. You say, "Assume 30, since that is what C-lightning itself uses, and we have probably the lowest such limit among implementations in common use," but 30 HTLCs is a rather high limit. My C-Lightning node has automatically force-closed a channel with only 4 outstanding HTLCs, and it cost me over $5 in chain fees, once all was said and done, so I surely don't want to allow 30 outstanding HTLCs on any channel; that'd be a recipe for going broke. I have set max-concurrent-htlcs=5 to limit my potential losses. If other node operators are adopting the same strategy, then your estimate of 30 HTLCs per channel doesn't hold water. Besides, you're assuming that no other multi-part payments are in progress to the destination node concurrently, but that is unlikely to be true for high-volume destinations.

Shouldn't C-Lightning strive to minimize the number of concurrent HTLCs that any given multi-part payment generates? Each additional HTLC costs additional money (base fee) and carries a non-zero (and rather high) probability of triggering a channel force-closure and costing the user many dollars in chain fees. Your current strategy is just too expensive.

[ZmnSCPxj]

30 here is the maximum we will split later. See ASSUMED_REASONABLE_HTLCS_PER_CHANNEL instead for the limit on the presplit. This is the initial limit on the number of splits, and comes to 30 / 3 = 10. We do the presplit for up to 10 splits per channel, which gives us some slack later for adaptive splitting.

[ZmnSCPxj]

Moved into a new paymod, since @whitslack brings up that maybe we should not really split early, so maybe we might decide to remove presplitter later. Also, toned down the ASSUMED_MAX_HTLCS_PER_CHANNEL.

[cdecker]

This is rather less convoluted than what I expected to be honest, the paymod structure seems to be working :-)
I was about to argue that it'd be better if we limit the overall number of HTLCs the presplit modifier is allowed to create (using either the fibonacci series in #3951 or the simpler power of 16 proposal that doesn't have code yet), but since this is a rather nicely self-contained limiter I think we should keep it.

Might come in handy to have another early abort signal.

[ZmnSCPxj]

I was about to argue that it'd be better if we limit the overall number of HTLCs the presplit modifier is allowed to create

I would argue that this is different from the presplit:

• The presplit limit is for the expected network connectivity, i.e. the average connectivity of hop nodes.
• This limit is for the destination connectivity.

We already have a limit for the source connectivity, that is the only limit we have.

We should limit to the lowest of all three limits: source, network, destination.

The source or the destination could be unusually low-connectivity compared to the network, which is why the presplit limit (which represents the expected limit of the network) is different from the destination limit (which we derive from the number of channels we know to the destination).

[ZmnSCPxj]

Rebased (especially with paymod_log), converted max_htlcs to u32. I still think separating the code out is good encapsulation, even if it currently only has one caller. The why string is required as well, as this is the justification we have to limit the number of HTLCs we will try.

[cdecker]

Rebased on top of master. LGTM 👍

ACK db979fe

[rustyrussell]

Rebased onto #4035

[cdecker]

ACK 08a22b7