Allow nodes to overshoot the MPP total_msat when paying

[TheBlueMatt]

When a node retires a failed path as part of a larger MPP payment, the node may wish to use a path which is constrained by an htlc_minimum_msat value. In this case, the node is forced to overpay, likely overshooting the total_msat it set in the earlier onions for the same MPP payment.

There are two possible solutions to this - either allow the total_msat value to change in later HTLCs or allow the node to (slightly) overshoot the total_msat value.

Allowing total_msat to change across HTLCs is nontrivial to implement - HTLCs may arrive out-of-order, causing the receiving node to have to track all seen total_msat values and accept a set of HTLCs which meet any of the seen total_msat values.

Instead, this commit changes the MPP logic to simply allow a sender to overshoot the stated total_msat.

Sadly the backwards-compatibility story for this is not great. There doesn't seem to be a good way to resolve this issue in a backwards-compatible way. Instead we just bite the bullet and make the incompatible change, hoping the overshooting is rare enough that it's not a major issue.

[joostjager]

Can't this be fixed via the change described in #1025?

So allow the htlc amount to exceed the amt_to_forward for the final node. That way the amount that is overpaid falls more in the category of fees paid and the mpp logic can be left alone?

[rustyrussell]

If you overpay amt_to_forward then the prior node can steal the excess funds (at least in theory).

Better to loosen this requirement, which was made because nobody could think of a reason to allow overpaying...

[joostjager]

If you overpay amt_to_forward then the prior node can steal the excess funds (at least in theory).

Does this matter at all? In the end, it's just costs to complete the payment from the sender point of view and taken into account during pathfinding. If the sender is intending to deliver more sats to the final destination, this will be locked into amt_to_forward and total_msat and the excess can't be stolen.

As mentioned in #1025 (comment), "stealing" can always happen at the prior hops if the sender was required to raise the htlc amount to meet a min_htlc policy. You could argue that having the same logic across the whole route is more consistent too.

Another reason why I think that relaxing the check on amt_to_forward rather than total_msat is good, is that it opens up the possibility for the final hop to charge inbound fees. If inbound fees would need to be added to amt_to_forward, it will be unclear on the htlc level whether sufficient inbound fees were paid. Only when the total set comes together, it may show that the total amount is short of total_msat (after deduction of the inbound fees). It may get complicated to decide which htlcs should be failed back with FinalIncorrectHtlcAmount.

[TheBlueMatt]

Does this matter at all? In the end, it's just costs to complete the payment from the sender point of view and taken into account during pathfinding. If the sender is intending to deliver more sats to the final destination, this will be locked into amt_to_forward and total_msat and the excess can't be stolen.

Yes, if nothing else it incentivizes nodes to try to steal repeatedly, which may reveal if the next hop is the last hop, and also delayed payments. This is not good, and I don't see any reason to enable this given there doesn't seem to be a motivation?

As mentioned in #1025 (comment), "stealing" can always happen at the prior hops if the sender was required to raise the htlc amount to meet a min_htlc policy. You could argue that having the same logic across the whole route is more consistent too.

That's just another way to charge fees, you could also increase your fees. There is a huge difference here.

Another reason why I think that relaxing the check on amt_to_forward rather than total_msat is good, is that it opens up the possibility for the final hop to charge inbound fees.

I don't see why this is related? The scheme you landed on enables this just fine without enabling the above concerns?

[joostjager]

Yes, if nothing else it incentivizes nodes to try to steal repeatedly, which may reveal if the next hop is the last hop, and also delayed payments. This is not good, and I don't see any reason to enable this given there doesn't seem to be a motivation?

I don't think it reveals if the next hop is the last hop, because for intermediate nodes you could currently already do the same.

Suppose a sender wants to send 80 sat over channels A->B->C for which A has a min_htlc routing policy of 100 sat. For simplicity, routing fees are all zero. The sender has to instruct A to forward 100 sat to satisfy the routing policy. They also instruct B to forward 80 sat to C. In this case, A could attempt to first forward 90 sat (violating its own routing policy that nobody else enforces) and see if B would accept that. In this case, B would.

If this possibility is deemed to be problematic, it may be an option to fix it for every hop by adding a htlc_amt field to the tlv onion (in addition to amt_to_forward). This will allow each hop (not just the last one) to detect that its predecessor is trying to steal fees.

I don't see why this is related? The scheme you landed on enables this just fine without enabling the above concerns?

Because as I said, you won't be able to keep apart the inbound fee paid from the intended contribution to the mpp set.

[t-bast]

I believe that @joostjager is right, we should go further and allow receiving a higher amount_msat in htlcs than the onion payload's amt_to_forward (and also total_msat as this PR does), otherwise we make it very easy to probe that a node is the final recipient (see this comment for a concrete example).

If you overpay amt_to_forward then the prior node can steal the excess funds (at least in theory).

But the spec and the implementations already allow intermediate nodes to overpay since we only check that amount_msat - fee >= amt_to_forward (from Bolt 4).

Since we allow this behavior for intermediate nodes (which I believe we have to do), we must allow it for the final node as well to fix the probing issue, don't we?

[TheBlueMatt]

I don't think it reveals if the next hop is the last hop, because for intermediate nodes you could currently already do the same.

My point was this - if we make the change you propose, and a forwarding node attempts to forward a lower-than-specified value, and it succeeds, they know with very very high confidence they are the last hop.

Suppose a sender wants to send 80 sat over channels A->B->C for which A has a min_htlc routing policy of 100 sat. For simplicity, routing fees are all zero. The sender has to instruct A to forward 100 sat to satisfy the routing policy. They also instruct B to forward 80 sat to C. In this case, A could attempt to first forward 90 sat (violating its own routing policy that nobody else enforces) and see if B would accept that. In this case, B would.

No, B would not because from its perspective the payment didn't pay sufficient fee so it would reject it.

If this possibility is deemed to be problematic, it may be an option to fix it for every hop by adding a htlc_amt field to the tlv onion (in addition to amt_to_forward). This will allow each hop (not just the last one) to detect that its predecessor is trying to steal fees.

I'm super confused now - that htlc_amt field, as far as I understand, is exactly what amt_to_forward is, or amt_to_forward + fees. Let's chat on the call today cause I think we're wayyyy off in communicating here.

[TheBlueMatt]

But the spec and the implementations already allow intermediate nodes to overpay since we only check that amount_msat - fee >= amt_to_forward (from Bolt 4).

Right, so there's two conversations - theres overpaying in general, and also what was discussed in #1025 where the sending node setting amt_to_forward to the value they want to pay, not the amount they did pay. Anyway, we should really discuss this on the call cause its now a mess of different ideas and conversations.

[t-bast]

I added these additional changes on top of this PR in #1032

[cdecker]

I wonder how often this is really an issue, given that we have to either relax amounts across all channels or keep them all exact. As far as I could tell from checking a couple of big nodes I couldn't find any node that had set a value different from 1sat. At least having an estimate of impact could guide us as to how urgent it is.

[TheBlueMatt]

Learned at the meeting today that actually LDK is the only implementation that is willing to take a path with an HTLC-minimum counting as excess fee, ie overpaying to meet a limit. Given many nodes use 1sat for their minimum this is probably useful to send < 1sat payments, assuming you're willing to pay the extra few msat to get there. That said, I'm not sure why that would matter for MPP, hopefully you're not MPP'ing a 999msat payment.

There was some discussion in the meeting of going the other way and no longer allowing overpayment at all, but it seemed like the meeting was otherwise positive here, as a "just do it cause its easy", but if there's agreement on removing overpayment entirely then we'd have to not do this (or #1032, which would require a more major change to fix the probing issue mentioned there).

[t-bast]

Actually this is already eclair's behavior, and has been for quite a while!

[joostjager]

As the spec currently is, Lightning can't be used for applications where only a strict amount must be accepted. I don't think that's good.

[TheBlueMatt]

I think we should have that conversation separately. That would require breaking existing nodes in several different ways (IIUC every sender except lnd may overshoot in some cases, and CLN does it very often), so we'd have to change a lot to get there - we'd also have to overhaul #1032 and add new onion fields to fix the probing issue, IIUC. If we do want to move towards strict acceptance, that seems like something that we'll need to roll out over a very long time horizon, with several dependent changes.

The changes here and in #1032 seem pretty simple and straight-forward, fix bugs today, and don't (IMO) require a super slow roll-out but can just be done today.

[joostjager]

The only thing is that if we decide to allow strict payment amounts, this PR made a step in the opposite direction. This is indeed true for #1032 too.

[TheBlueMatt]

To some extent. If we decide to go the "no overpayment allowed" route, we'll need to add new fields to the onion to address #1032. Whether those new fields are added against lightning today or lightning with 1032, I don't think the effort or complexity changes. Its a bit different for this PR, because if we go the "no overpayment" route we'd probably just fix the total_msat.

As for the "do we want to do that" question, I think the CLN privacy work is a super compelling reason to scream no. CLN randomizes values somewhat to hide the path selected and if you're the second-to-last hop or not. I think that is an incredibly compelling reason to want to be able to overpay the last node, privacy for "where am I in the path" questions today is really broken, and I'm more than happy to force people to handle extra money to improve the state of affairs there.

[TheBlueMatt]

Squashed and fixed a typo @t-bast caught.

[rustyrussell]

Ack.

[t-bast]

LGTM

[Roasbeef]

LGTM 🪁

[halseth]

Post-merge ACK.

[joostjager]

Now that we have #1032, shouldn't amount_msat be changed to the final amt_to_forward? To ensure that the sender remains in control of the set total?

[t-bast]

We could, that's a good point!

[joostjager]

I suppose it is still at the discretion of the receiver, but a premature release of the preimage may cause the receiver to receive less (although still meeting total_msat). With a premature release, routing nodes may short-cut pending htlcs.

So incentive-compatible is to only count the minimum towards total_msat, which is amt_to_forward?

[t-bast]

Yes that makes sense, don't hesitate to open a follow-up PR to fix that!

[halseth]

Same discussion as we had here? #1032 (comment)

[joostjager]

Yes, but perhaps new is that we want to at least recommend the most incentive-compatible way in the spec?

[t-bast]

I agree, I opened #1040 to fix that.