Blockstream / greenlight

Build apps using self-custodial lightning nodes in the cloud
https://blockstream.github.io/greenlight/getting-started/
MIT License
115 stars 27 forks source link

Allow changing forward_amount (lower it) in htlc_accepted hook #64

Closed roeierez closed 1 year ago

roeierez commented 1 year ago

As LSP we open channels just in time when forwarding payments. The flow is roughly as the following:

Upon receiving the htlcs that need to be forwarded to the receiver the LSP:

The fee is applied by lowering the forward amount for each htlc which is currently not supported.

cdecker commented 1 year ago

Looking into this yesterday I had a realization: we don't need to change anything in CLN, and it can be fully implemented in an LSP server plugin and a corresponding LSP client plugin. I went ahead and put together a brief proof-of-concept here.

The trick is rather simple:

This means that the HTLC verification step in lightningd happens to match up, and we get to the htlc_set tracking step, which then triggers the resolution of the invoice, where its validation also matches up.

Notice that there are still some open questions here:

Breez-specific

My guess is that Breez currently uses a pre-negotiation model, in which the invoice amounts are replicated on the Breez server, thus the plugin can likely determine whether it is an LSP payment or not based on some property (I guess a suffix in the invoice would be sufficient) and assume that the amt_to_forward is correct. I'm still unsure if we're allowed to have a mismatch between total_msat and the invoice amount, but I'll check, so for now we just make sure they match exactly (so the LSP needs to forward exactly that as a sum of HTLCs).

If you agree that this is a first workable solution I can get started with the LSP-client plugin for GL, based on the information in the invoice. We can always add a more complex negotiation in a second step.

Notice that the one big downside here is that the LSP client will accept any invoice that was marked as an LSP-enabled invoice, independently of whether the LSP had to open a channel or not (can happen if we create multiple invoices before paying the first one: all invoices will have the reduced amount, but we can then overpay with the original amount so 🤷).

roeierez commented 1 year ago

@cdecker I think you nailed it. Here are my two cents for moving forward.

  • Have a pre-negotiation phase (as part of the small invoice being created) storing these parameters on the LSP
  • Have an inline negotiation using custommsg to ask the client if they are ok with receiving X msat lower total amount.

The first option is currently implemented in Breez. The payee send the payment hash and amount to the LSP that are used to differentiate htlcs that require interceptions from those that are just "simple forwards" The second option is great for removing this "static" step and allow the LSP determine just in time if opening a channel is really needed (based on liquidity). It will require though (correct me if I am wrong) the client plugin to create/replace an existing invoice once the amount is determined.

  • The example does not have support for MPP payments since it is a static example. In reality the LSP would likely have to track parts and how much it already charged so that the invoice amount is matched up or exceeded. For this reason I'd suggest we round down on operations that determine the invoice amount, and round up when it comes to the forward amount. This will result in a safe system that may underpay the LSP by < 1msat * number_of_parts, which I think is acceptable.

Right. We chose a simple strategy that reduce proportionally the fee amount from every htlc which freed the implementation from tracking the paid amount per payment.

  • We may need a negotiation phase for keysend, unless the LSP always charges proportionally from each HTLC.

I think to support open channel with keysend (we don't support it right now in Breez) we must have a just in time negotiation phase regardless of how LSP charges since the base assumption is that the client must have an invoice to settle the lower amount (or I am wrong here?)

Notice that the one big downside here is that the LSP client will accept any invoice that was marked as an LSP-enabled invoice, independently of whether the LSP had to open a channel or not (can happen if we create multiple invoices before paying the first one: all invoices will have the reduced amount, but we can then overpay with the original amount so 🤷).

Right and this is indeed the downside of not having just in time negotiation. In reality our LSP implementation will change the htlcs only if it opens a channel so it is not ideal (currently allows the LSP to take fee without opening a channel) but is a limitation of the "static" negotiation I think.

Moving forward @JssDWt wants to go for a POC on our side which is great and might give us the confident to progress. Should not take too long. IMO we can go for the "static" negotiation as a first milestone which will give us a workable solution relatively fast and then we can go for the excited "just in time" negotiation as a second milestone. LMK what you think of course.

JssDWt commented 1 year ago

The trick is rather simple:

  • The LSP changes the amt_to_forward in its own payload to the lower amount we want to forward. This causes lightningd to just forward that much.
  • The client node would then receive an HTLC whose payload does not match in two ways: amt_to_forward does not match the amount in the HTLC, and the total_msat in the payment_data does not match (or exceed) the invoice amount. Either of these causes the HTLCs to fail, and therefore the invoice would never be resolved as a whole either.

    • The client just has to also opt-into the lower amount (the mismatch comes from the sender having one invoice and the recipient having another), by adjusting its own payload to match the HTLC value with the amt_to_forward (misnamed field in the spec) and the total_msat at the end of the payment_data to match or exceed the invoice amount.

Update: I verified this solution works.

JssDWt commented 1 year ago

For reference here's the change required for Breez. Pretty small change overall. Most changes were in the tests. https://github.com/breez/lspd/pull/55